Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add frontend #186

Merged
merged 41 commits into from
Feb 1, 2022
Merged

Add frontend #186

merged 41 commits into from
Feb 1, 2022

Conversation

DNepovim
Copy link
Collaborator

@DNepovim DNepovim commented Dec 8, 2021

No description provided.

@DNepovim DNepovim requested a review from marekdedic December 8, 2021 10:47
@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #186 (cd76a76) into master (06c8f46) will not change coverage.
The diff coverage is 100.00%.

❗ Current head cd76a76 differs from pull request most recent head 884a2ed. Consider uploading reports for the commit 884a2ed to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master      #186   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          210       210           
  Branches        54        54           
=========================================
  Hits           210       210           
Impacted Files Coverage Δ
packages/collector/src/getGlobalConfig.ts 100.00% <100.00%> (ø)
packages/collector/src/run.ts 100.00% <100.00%> (ø)

@marekdedic
Copy link
Collaborator

Ahoj,
díky za PR.
Koukám, že tam máš vlastní package.json, zatímco já jsem nechal jeden globální a že to dělá problémy v buildu. Můžeme buď všechno dát do jednoho globálního, nebo každý z těch 2 projektů bude mít dependence zvlášť a v rootu repa by pak asi byl package.json, kde by byly jen skripty?

Asi je mi to vcelku jedno, když jsem dělal ten collector, tak jsem nad tím chvilku uvažoval a pak jsem narazil na věci jako Lerna a přišlo mi to nad moji úroveň. Každopádně pokud by ti to tak přišlo lepší, tak tomu collectoru udělám vlastní package.json, možná bych tě pak poprosil o pomoc. Dík

@marekdedic
Copy link
Collaborator

Koukám, že tam máš duplikované interfaces z toho collectoru - nenapadá tě, jak to čistě udělat bez duplikace?

@DNepovim
Copy link
Collaborator Author

DNepovim commented Dec 9, 2021

Ač mi připadá čistší, aby frontend i collector měly každý svůj package.json, tak jsem to nejdřív zkusil nacpat vše do jednoho, páč jsem myslel, že to bude jendodušší. Ale ty závislosti se tam dost bily. Říkám si jestli vůbec potřebujeme nějaký package.json v rootu a ta Lerna nám podle mě také s ničím nepomůže. Teď jsem udělal z collectoru a frontendu samostatné projekty a přistupuji z CI.yml přímo do nich.

Myslíš, že je to proti něčemu?

Copy link
Collaborator

@marekdedic marekdedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config.json prosím nechat v rootu

packages/frontend/.gitignore Show resolved Hide resolved
packages/frontend/README.md Show resolved Hide resolved
packages/frontend/public/index.html Outdated Show resolved Hide resolved
packages/frontend/public/index.html Outdated Show resolved Hide resolved
packages/frontend/public/manifest.json Outdated Show resolved Hide resolved
packages/frontend/public/manifest.json Show resolved Hide resolved
packages/frontend/src/index.tsx Outdated Show resolved Hide resolved
packages/frontend/src/testData.ts Show resolved Hide resolved
Copy link
Collaborator

@marekdedic marekdedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Díky, detaily Reactu moc neznám, tak jsem spíš komentoval drobnosti v konfiguraci projektu. Ale prošel jsem to, celkově to vypadá OK, prosím o nějaké dílčí změny podle komentářů, případně mi klidně vysvětli, že to tak má být a já tomu jen nerozumím :D

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
packages/collector/jest.config.ts Outdated Show resolved Hide resolved
packages/collector/package.json Outdated Show resolved Hide resolved
packages/frontend/package.json Outdated Show resolved Hide resolved
packages/frontend/tsconfig.json Outdated Show resolved Hide resolved
packages/frontend/tsconfig.json Outdated Show resolved Hide resolved
packages/frontend/tsconfig.json Outdated Show resolved Hide resolved
packages/frontend/tsconfig.json Show resolved Hide resolved
packages/frontend/package.json Outdated Show resolved Hide resolved
@marekdedic
Copy link
Collaborator

marekdedic commented Dec 12, 2021

Přidávám pro trackování dříve popsaného

  • Renderování markdownu na hlavní stránce
  • Vizuální odlišení Projektu a poptávky
  • Hlavní stránka má v adrese poptavky-fe, další již ne - něco je někde asi trochu špatně s routováním?
  • Unikátní URL každého projektu i poptávky
  • Sjednotit terminologii - nechceme používat anglické issue, ideální asi "Poptávka" - že, @DeetsCZ ?
  • Co udělat, aby hlavní strana nebyla nekonečná propast na poptávky? (asi zatím nic, v budoucnu třeba Volitelná funkčnost: filtrování podle tagu na frontendu #190 )

@marekdedic
Copy link
Collaborator

Opravíš prosím i další GH workflows? Jsou tam ještě 2, které budou přesunem package.json určitě rozbité... Díky

@marekdedic
Copy link
Collaborator

No a úplně další pandořina skříňka jsou testy - pokud jsem něco nepřehlédl, tak ten frontend není otestovaný vůbec. Chtěl bych #157 , ale asi si to úplně netroufnu udělat, pokud to nějaký nekompatibilní update může složit - a to bez testů moc nepoznáme.

Upřímně se ve frontend testování moc nevyznám, v skaut/shared-drive-mover#722 si trochu hraju s testing-library, ale tam je to místo Reactu se Svelte a to je celkově ještě málo vyvinutý ekosystém... Asi nechci být úplně tvrďák a požadovat 100% coverage, jako u collectoru, ale alespoň nějaké end-to-end testy by to podle mě mít mělo... Co myslíte?

@marekdedic marekdedic mentioned this pull request Dec 14, 2021
1 task
@DNepovim
Copy link
Collaborator Author

No a úplně další pandořina skříňka jsou testy - pokud jsem něco nepřehlédl, tak ten frontend není otestovaný vůbec. Chtěl bych #157 , ale asi si to úplně netroufnu udělat, pokud to nějaký nekompatibilní update může složit - a to bez testů moc nepoznáme.

Upřímně se ve frontend testování moc nevyznám, v skaut/shared-drive-mover#722 si trochu hraju s testing-library, ale tam je to místo Reactu se Svelte a to je celkově ještě málo vyvinutý ekosystém... Asi nechci být úplně tvrďák a požadovat 100% coverage, jako u collectoru, ale alespoň nějaké end-to-end testy by to podle mě mít mělo... Co myslíte?

Přidal jsem nějaké základní unit testy. end-to-end testy psát nebudu. Vzhledem k jednoduchosti aplikace a očekávané návštěvnosti mi to připadá jako házení hrachu.

@marekdedic
Copy link
Collaborator

Díky moc za úpravy, zvlášť za testy. Souhlasím, že end-to-end je v tomhle případě overkill...

@DNepovim DNepovim requested a review from marekdedic January 20, 2022 21:36
@marekdedic
Copy link
Collaborator

Díky, trochu jsem učesal GH actions a věci v tom collectoru. Přijde mi, že to je v tuhle chvíli je to hotový, jen ještě přemýšlím, jestli nejde nějak čistě deduplikovat interfaces...

@DNepovim
Copy link
Collaborator Author

Ach, tipoval jsem tě spíš na rebasera, než mergera... :D Nu což je to tvůj repozitář...

Udělám ještě něco s tím coverage ať to můžeme zavřít.

@marekdedic
Copy link
Collaborator

:D Záleží na tom, jak moc se mi s tím chce ... to... :D

Díky!

@marekdedic
Copy link
Collaborator

Ahoj,
zvládneš to prosím teda nějak postrčit za finišovou laťku? Díky

@DNepovim DNepovim merged commit 2cd7580 into master Feb 1, 2022
@DNepovim DNepovim deleted the add-frontend branch February 1, 2022 15:08
@DNepovim DNepovim restored the add-frontend branch February 1, 2022 15:20
@DNepovim DNepovim deleted the add-frontend branch February 1, 2022 15:23
@marekdedic marekdedic mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants